feat: Magisk module install#127
Conversation
Introduce Magisk-based installation support: add an abstract MagiskInstaller implementing install/uninstall/getInstallation logic for deploying patched APKs as Magisk modules (writes module.prop, copies patched APK to module, restarts/kills apps). Add platform-specific installers: AdbMagiskInstaller (ADB root) and LocalMagiskInstaller (local root via LocalShellCommandRunner, Closeable). Add Magisk-related constants (module paths, COPY_APK_TO_MODULE command, MAGISK_MODULE_PROP template) used by the installer.
| rm -f "$DIR/log" | ||
|
|
||
| { | ||
| echo "Induction check for $package_name" |
There was a problem hiding this comment.
What does induction check mean
There was a problem hiding this comment.
Basically making sure that the system is ready for the app to be injected/introduced into the system by overlaying on top of the original app
There was a problem hiding this comment.
Just a mix of both, right now that whole name has been replaced by the watchdog and STAGE_APK variables
There was a problem hiding this comment.
Standardized naming on these commits
secp192k1@041519a
secp192k1/revanced-manager@8f8efa5
| * | ||
| * @throws PackageNameRequiredException If the [Apk] does not have a package name. | ||
| */ | ||
| override suspend fun install(apk: Apk): RootInstallerResult { |
There was a problem hiding this comment.
Seems like this doesnt handle native libs mounting, like the extractNativeLibs utils function from earlier?
There was a problem hiding this comment.
Seems like this doesnt handle native libs mounting, like the extractNativeLibs utils function from earlier?
More detailed comment here #127 (comment)
But basically since we switched to PM for mounting, its not needed, it will be needed for bind-mounting though
There was a problem hiding this comment.
I think im not understanding the diff between bind and regular mount.
For mount installations (magisk also mounts no?) we always have to ensure the base apk is installed, with pm. Yes, if the patched apk introduces new libs those would have to be extracted since we dont invoke pm for the patched apk which does this step, but this would apply to mount and bind mount equally.
There was a problem hiding this comment.
I think im not understanding the diff between bind and regular mount.
For mount installations (magisk also mounts no?) we always have to ensure the base apk is installed, with pm. Yes, if the patched apk introduces new libs those would have to be extracted since we dont invoke pm for the patched apk which does this step, but this would apply to mount and bind mount equally.
Two different approaches:
a. RootInstaller (bind-mount): patched apk overlaid over stock apk path via mount bind. PM never sees a new install, it reuses the /libs/ it extracted from the stock apk at install time. New .so files added by a patch will throw UnsatisfiedLinkError
b. MagiskRootInstaller (module): patched apk installed fresh via pm install inside service.sh. PM handles the full install including lib extraction = no issue 😄
Native lib extraction is only a gap for the bind-mount path; Module install is already fine
| override suspend fun uninstall(packageName: String): RootInstallerResult { | ||
| logger.info("Uninstalling $packageName Magisk module") | ||
|
|
||
| val formattedPackageName = packageName.replace('.', '_') |
There was a problem hiding this comment.
In which case would . be unsafe to use here? Isnt
/my.pkg.name/ a valid folder name
There was a problem hiding this comment.
In which case would . be unsafe to use here? Isnt
/my.pkg.name/ a valid folder name
Yes its a valid folder name, but because formattedPackageName doubles as the Magisk module id in module.prop (id=revanced___FORMATTED_PKG__), which by convention should only contain alphanumeric characters and underscores
Since the module id must match the directory name under the modules dir (/data/adb/modules/), both are kept consistent and dot-free
There was a problem hiding this comment.
I see, so magisk says alphanumeric -> therefore folder has to as well. Hm, is this convention something magisk mentioned anywhere? Whats the reason for this convention? Its an internal name, users dont see it. I would prefer us to break this convention on the magisk side if it doesnt cause any issues there, for the sake of not having to do this quirky operation here
There was a problem hiding this comment.
is this convention something magisk mentioned anywhere? Whats the reason for this convention?
Docs are located here, Magisk/docs/guides.md
id has to match this regular expression:
^[a-zA-Z][a-zA-Z0-9._-]+$
ex: ✓ a_module, ✓ a.module, ✓ module-101, ✗ a module, ✗ 1_module, ✗ -a-module
This is the unique identifier of your module. You should not change it once published.
So yeah dots are allowed... Missed it completely, since they're allowed anyways, I just removed the entire formatter logic (remove dots to underscore logic)
secp192k1@d33b54c
| return RootInstallation( | ||
| INSTALLED_APK_PATH(packageName)().output.ifEmpty { null }, | ||
| patchedApkPath, | ||
| MOUNT_GREP(patchedApkPath)().exitCode == 0, |
There was a problem hiding this comment.
Any reason we still have the mounted boolean https://github.com/ReVanced/revanced-library/pull/127/changes#r3061197338
There was a problem hiding this comment.
Any reason we still have the mounted boolean https://github.com/ReVanced/revanced-library/pull/127/changes#r3061197338
Lmao... 2d19f45
Also the val above as useless so I moved it inside the root installation call
| #!/system/bin/sh | ||
| pm uninstall "__PATCHED_PKG__" | ||
| rm -f "/data/adb/revanced/__PKG_NAME__.apk" | ||
| rm -f "/data/adb/service.d/revanced_handle_disabled___FORMATTED_PKG__.sh" |
There was a problem hiding this comment.
Shouldnt the module uninstall script delete itself too?
There was a problem hiding this comment.
Shouldnt the module uninstall script delete itself too?
Nope. Magisk runs the uninstall script and then removes the entire module directory itself, so the script is cleaned up automatically. The explicit rm -f for the handle-disabled script is necessary precisely because that file lives outside the module directory at /data/adb/service.d/ : Magisk will never touch it
| cp /proc/sys/kernel/random/boot_id "${DIR}/.boot_token" | ||
|
|
||
| LOG="${DIR}/log" | ||
| MAX_LOG_LINES=200 |
There was a problem hiding this comment.
Why do we need logging here? Is it to display stuff in magisk manager?
There was a problem hiding this comment.
I think its fine to not have any logging code for the module script, like logging to magisk manager is fine, but no dump to a log file
There was a problem hiding this comment.
Why do we need logging here? Is it to display stuff in magisk manager?
No, this logging is strictly for the magisk module, since manager wouldn't be able to access the logs and to keep it organized by category (manager logs vs module logs) its also keeps the logs separated between multiple modules
I think its fine to not have any logging code for the module script, like logging to magisk manager is fine, but no dump to a log file
Any other alternative that comes to mind?
Also keep in mind, along side with my reply above, each log file has a MAX_LOG_LINES=200
There was a problem hiding this comment.
Well I would simply remove the log entirely, I understand its a nice to have for debugging purposes, but debugging code doesnt belong in production as a general rule of thumb principle. Debugging needs to be solved elsewise
There was a problem hiding this comment.
Well I would simply remove the log entirely
| # (install-create/write/commit) and is less prone to early-boot binder failures. | ||
| # NOTE: On Xiaomi devices (MIUI/HyperOS), pm install may still fail due to | ||
| # package verification restrictions. Manual install via ReVanced Manager | ||
| # may be required in that case. |
There was a problem hiding this comment.
Insane lol, how do we handle that in manager tbh or like how tf do you prompt the user to install the apk on Xiamoi programatically, ideally library can handle Xiomi installation business logic by itself
There was a problem hiding this comment.
Inside lol, how do we handle that in manager tbh or like how tf do you prompt the user to install the apk on Xiaomi programatically, ideally library can handle Xiomi installation business logic by itself
How basically everyone else solves this issue, by the phone props, for example:
[ro.fota.oem]: [Xiaomi]
[ro.product.bootimage.manufacturer]: [Xiaomi]
[ro.product.manufacturer]: [Xiaomi]
[ro.product.odm.manufacturer]: [Xiaomi]
[ro.product.product.brand]: [Xiaomi]
[ro.product.product.manufacturer]: [Xiaomi]
[ro.product.system.brand]: [Xiaomi]
[ro.product.system.manufacturer]: [Xiaomi]
[ro.product.system_ext.brand]: [Xiaomi]
[ro.product.system_ext.manufacturer]: [Xiaomi]
[ro.product.vendor.manufacturer]: [Xiaomi]
[ro.product.vendor_dlkm.manufacturer]: [Xiaomi]
Note: Remember that these are keys from my device with the value specifically being Xiaomi, some keys like ...vendor_dlkm... might not exist on other Xiaomi devices
Also this will almost always be a hassle to completely fix since there isnt a good and reliable way to tell MIUI and HyperOS apart (without a lot of checks)
There was a problem hiding this comment.
You could for example join all props and do a "contains Xiaomi" check.To be honest im not really fond of us handling xiaomi bugs in revanced, or even considering them here, its more of something xiaomi would have to solve on their side
Co-authored-by: oSumAtrIX <johan.melkonyan1@web.de>
…anced-library into feat/magisk-module
| * | ||
| * 3. Some potential issue is that writing to [system app's path]/lib would fail on read-only | ||
| * system partition (/system/app, /product/app, etc) | ||
| * Right now it's assuming it can write to it - Wrong! |
There was a problem hiding this comment.
Cant we just mount to that directory instead of writing?
There was a problem hiding this comment.
Cant we just mount to that directory instead of writing?
secp192k1@99afcc7
Like this?
4 participants
Closes #127
Note: I had to rollback the AGP version during local development and testing to
8.13.2since there is a version mismatch. But this shouldn't be an issue once they match